Conversation
81521eb to
00e7269
Compare
.cursorignore
Outdated
There was a problem hiding this comment.
Is this accidentally committed ?
There was a problem hiding this comment.
removed .cursorignore file
| "ios": { | ||
| "supportsTablet": true, | ||
| "bundleIdentifier": "com.AEPSampleAppNewArchEnabled" | ||
| "bundleIdentifier": "com.adobe.MessagingDemoAppSwiftUI" |
There was a problem hiding this comment.
this change should be reverted before merging
There was a problem hiding this comment.
reverted back to com.AEPSampleAppNewArchEnabled
There was a problem hiding this comment.
i think we dont need this ..
There was a problem hiding this comment.
earlier we were not adding dist folder to commit. any reason that why have added it to our version control now
There was a problem hiding this comment.
We use this to distribute the binaries to the beta testing team while we’re unable to publish to npm.
| onPress?.(interactId); | ||
| if (actionUrl) { | ||
| try { | ||
| Linking.openURL(actionUrl); |
There was a problem hiding this comment.
We should use Linking.canOpenURL() in order t o validate the links .
const supported = await Linking.canOpenURL(actionUrl);
if (supported) {
Linking.openURL(actionUrl);
} else {
console.warn(...)
}
There was a problem hiding this comment.
Added Linking.canOpenURL() logic to Button.tsx for link validation
|
|
||
| const Button: React.FC<ButtonProps> = ({ | ||
| actionUrl, | ||
| id, |
There was a problem hiding this comment.
The id prop is declared but not utilized anywhere in the component.
There was a problem hiding this comment.
removed unnecessary id prop from Button.tsx
| ...props | ||
| }) => { | ||
| const theme = useTheme(); | ||
| const handlePress = useCallback(() => { |
There was a problem hiding this comment.
The handlePress callback currently does not accept the GestureResponderEvent emitted by Pressable. As a result, onPress consumers never receive the event, even though the component’s type signature indicates that the event should be forwarded.
This creates an inconsistency between the API contract and the implementation.
Proposed fix:
const handlePress = (event: GestureResponderEvent) => {
onPress?.(interactId, event);
if (actionUrl) {
Linking.openURL(actionUrl).catch(...);
}
};
This keeps the API consistent and prevents unexpected undefined events in parent components.
There was a problem hiding this comment.
Added GestureResponderEvent to handlePress callback in Button.tsx
| title: string; | ||
| onPress?: (interactId?: string, event?: GestureResponderEvent) => void; | ||
| interactId?: string; | ||
| textStyle?: (TextStyle | undefined) | (TextStyle | undefined)[]; |
There was a problem hiding this comment.
textStyle?: TextStyle | TextStyle[];
It can be simplified to this
There was a problem hiding this comment.
changed to textStyle?: StyleProp<TextStyle>; to match Reacy Native’s style prop and allow arrays, null/undefined/false, and registered/animated styles without type errors.
| import { PropsWithChildren } from "react"; | ||
| import { StyleSheet, View } from "react-native"; | ||
|
|
||
| const CenteredView = ({ children }: PropsWithChildren) => ( |
There was a problem hiding this comment.
PropsWithChildren needs a generic type (e.g. {}) — otherwise it becomes any.
So it can be written like this
PropsWithChildren<{}>;
There was a problem hiding this comment.
changed to PropsWithChildren<ViewProps>
|
|
||
| const styles = StyleSheet.create({ | ||
| container: { | ||
| flex: 1, |
There was a problem hiding this comment.
Just to clarify: this will occupy the entire screen — is that the intent? If so, it seems we should be using or creating a FullScreenCenterView, since the current name is a bit confusing.
There was a problem hiding this comment.
changed CenteredView to be called FullScreenCenterView
| }, [actionUrl, interactId, onPress]); | ||
|
|
||
| return ( | ||
| <Pressable onPress={handlePress} style={style} {...props}> |
There was a problem hiding this comment.
An appropriate accessibilityRole should also be defined for this component.
There was a problem hiding this comment.
Added accessibilityRole = 'button', but also added it as an optional prop if user wants to change it to something else: accessibilityRole?: AccessibilityRole; in Button.tsx
|
|
||
| return ( | ||
| <ContentCardContainerProvider settings={settings}> | ||
| <Text accessibilityRole="header" style={[styles.heading, { color: headingColor }]}>{heading.content}</Text> |
There was a problem hiding this comment.
If heading is undefined, the component will throw when attempting to access heading.content.
Even though heading is marked as a required prop in contentSettings, it may still be missing at runtime.
If you want to add guard rails, you can safely render the heading using the following pattern:
{heading?.content ? (
<Text
accessibilityRole="header"
style={[styles.heading, { color: headingColor }]}
>
{heading.content}
</Text>
) : null}
There was a problem hiding this comment.
Applied the above pattern to heading in ContentCardContainer.tsx as a guard rail
| <FlatList | ||
| {...props} | ||
| data={displayCards} | ||
| extraData={refetch} |
There was a problem hiding this comment.
Quick question — why are we passing refetch to extraData?
My understanding is that extraData should be used for UI state that affects item rendering, but refetch is just a function and won’t trigger meaningful re-renders.
There was a problem hiding this comment.
removed extraData={refetch} from the ContentCardContainer, it was being used as a workaround, but not needed anymore
| } | ||
| CardProps?.listener?.(...args); | ||
| }} | ||
| style={[ |
There was a problem hiding this comment.
It can also be written like this
style={isHorizontal ? [styles.horizontalCardStyles, { width: Math.floor(windowWidth * 0.75) }] : undefined}
There was a problem hiding this comment.
rewritten to
style={
isHorizontal
? [styles.horizontalCardStyles, { width: Math.floor(windowWidth * 0.75) }]
: undefined
}
| return ( | ||
| <ContentCardContainerProvider settings={settings}> | ||
| <Text accessibilityRole="header" style={[styles.heading, { color: headingColor }]}>{heading.content}</Text> | ||
| <FlatList |
There was a problem hiding this comment.
We should implement a keyExtractor for this FlatList so it can render more efficiently and avoid unnecessary re-renders.
There was a problem hiding this comment.
Added keyExtractor={(item: { id: string }) => item.id} to this FlatList
|
|
||
| const [dismissedIds, setDismissedIds] = useState(new Set()); | ||
|
|
||
| const headingColor = useMemo(() => colorScheme === 'dark' ? '#FFFFFF' : '#000000', [colorScheme]); |
There was a problem hiding this comment.
can we move the color string to constants
There was a problem hiding this comment.
moved to usage of useTheme colors constant
| const { content: contentSettings } = settings; | ||
| const { capacity, heading, layout, emptyStateSettings } = contentSettings; | ||
|
|
||
| const [dismissedIds, setDismissedIds] = useState(new Set()); |
There was a problem hiding this comment.
As it is a typescript file we should add proper types for state
For ex:
useState<Set<string | number>>(new Set())
There was a problem hiding this comment.
changed to const [dismissedIds, setDismissedIds] = useState<Set<string>>(new Set());
| return ( | ||
| <ContentCardContainerProvider settings={settings}> | ||
| <Text accessibilityRole="header" style={[styles.heading, { color: headingColor }]}>{heading.content}</Text> | ||
| <FlatList |
There was a problem hiding this comment.
Can you confirm if this has been manually tested by rotating the device from portrait to landscape, especially for the horizontal FlatList case?
useWindowDimensions should recalculate the width automatically, but I just want to make sure none of the styles break when the orientation changes.
| surface, | ||
| style, | ||
| CardProps, | ||
| refetch, |
There was a problem hiding this comment.
Quick question: why didn’t we wire refetch to the FlatList onRefresh prop? Is there a reason we handled refresh differently?
There was a problem hiding this comment.
removed refetch logic from ContentCardContainer as it is not needed anymore
| <ContentCardView | ||
| template={item} | ||
| {...CardProps} | ||
| listener={(...args) => { |
There was a problem hiding this comment.
We should avoid creating a new inline listener inside renderItem for every item. This causes unnecessary re-renders and prevents ContentCardView from taking advantage of memoization.
If ContentCardView passes the card's id when firing the event (e.g., listener(event, item.id)), we can use a single shared callback:
const handleCardEvent = useCallback((event, itemId, ...rest) => {
if (event === "onDismiss") onDismiss(itemId);
CardProps?.listener?.(event, itemId, ...rest);
}, [onDismiss, CardProps]);<ContentCardView
template={item}
{...CardProps}
listener={handleCardEvent} // shared, stable listener
/>This avoids per-item function creation and improves FlatList performance.
There was a problem hiding this comment.
added the usage of a new single shared callback:
const handleCardEvent = useCallback(
(event?: ContentViewEvent, data?: ContentTemplate, nativeEvent?: any) => {
if (event === 'onDismiss' && data?.id) {
setDismissedIds((prev) => {
const next = new Set(prev);
next.add(data.id as any);
return next;
});
}
CardProps?.listener?.(event, data, nativeEvent);
},
[CardProps]
);
| image={emptyStateSettings?.image?.[colorScheme ?? "light"]?.url ?? ''} | ||
| text={ | ||
| emptyStateSettings?.message?.content || | ||
| "No Content Available" |
There was a problem hiding this comment.
Should localise it or use constants for rendering text
There was a problem hiding this comment.
added a constant for "No Content Available"
| const EmptyState: React.FC<EmptyStateProps> = ({ image, text }) => { | ||
| return ( | ||
| <CenteredView> | ||
| <Image source={{ uri: image }} style={{ width: 120, height: 120, padding: 10 }} resizeMode="contain"/> |
There was a problem hiding this comment.
Just a quick question: Should we add fallback image or not for network failure cases
Update tutorial doc for content card
update Content card customization guide
Update doc with Templates layout diagrams
Add useContentCardUI hook
update some format and wording
update doc
update doc
update doc format
update contentCardCustomizationGuide file name
This reverts commit 02461a4.
* Add unread UI implementation Add unread UI implementation * update the unread icon mock example update the unread icon mock example * Use useContainerSettigs hook Use useContainerSettigs hook and update the dist files * update unreadIcon for error handling update unreadIcon for error handling * Update with review comments Update with review comments * fix the building errors for sample app and update the sample app fix the building errors for sample app and update the sample app * update with review comments update with review comments * update for new comments update for the new comments * Update test Update test * Update metro.config and dist files Update metro.config and dist files * Update few UI color in the sample app Update few UI color in the sample app * update the code to automatically refetch the content card update the code to automatically refetch the content card * Revert "update the code to automatically refetch the content card" This reverts commit f8e1f1a. * update read status from native update read status from native * Add isRead to contentTemplate constructor Add isRead to contentTemplate constructor
* adding content card container ui component * removing console logs * removing templateType & capacity logic, fixing up styling * addressing PR feedback * more styling adjustments * removing trailing space * updating metro file * adding some basic unit tests for content card container + removing duplicate color scheme variable * removed unnecessary variable * fixing typing * addressing more pr feedback * renaming to CardProps --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
* adding unit tests for content card components * removing unused value --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
* adding content card container ui component * removing console logs * removing templateType & capacity logic, fixing up styling * addressing PR feedback * more styling adjustments * removing trailing space * updating metro file * adding some basic unit tests for content card container + removing duplicate color scheme variable * removed unnecessary variable * fixing typing * addressing more pr feedback * renaming to CardProps * adding capacity logic to content container * added chaining to the listener * adding back old app & track functionality * adding refetching & extraData --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
fix error message for missing button in the content card
* adding content card container ui component * removing console logs * removing templateType & capacity logic, fixing up styling * addressing PR feedback * more styling adjustments * removing trailing space * updating metro file * adding some basic unit tests for content card container + removing duplicate color scheme variable * removed unnecessary variable * fixing typing * addressing more pr feedback * renaming to CardProps * adding capacity logic to content container * added chaining to the listener * adding back old app & track functionality * adding refetching & extraData * adding initial changes for test app updates * fixing up test app for android * adding staging cards * adding fixes for messagin error/picture problems in android --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
* adding empty state * addressing pr feed * addressing pr feedback * fixing empty state in android * adding use callback & moving styling * changing to inbox for default --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
71ae1ef to
723ef9a
Compare
…into content-card-containers
* renaming all instances of container to inbox * adding unread status * adding more unit tests for on interaction * updating documentation * saving configuration * adding unread status & renaming inbox * fixing spacing * adding sudo * adding ci fix * updating enable corepack * undoing layout.tsx & app.json --------- Co-authored-by: Julia Kartasheva <jkartasheva@adobe.com>
Background -
We're implementing the Messaging Inbox feature with support for:
1. Container Settings - The SDK reads container payload and displays content according to supported settings configured via AJO Authoring UI (experience.adobe.com).
2. Content Card Templates - Support for displaying content cards with various templates (SmallImage, LargeImage, ImageOnly). See our usage documentation for implementation details.
Key Changes -
Most of the additions are located in the packages/messaging/src/ui/ folder, which contains:
• React components for Content Card rendering (ContentCardContainer, ContentCardView…)
• Supporting UI components (Button, DismissButton, UnreadIcon, EmptyState)
• Hooks for data fetching (useContentCardUI, useContentContainer…)
• Theme system and styling
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: